-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: React-Native package exports #4887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix: React-Native package exports #4887
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f9f283d:
|
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
49c76b3
to
2cecb47
Compare
@aryaemami59 can you summarize the package changes and why they improve things? |
2cecb47
to
25f7b50
Compare
25f7b50
to
c67da92
Compare
c67da92
to
2ebf48c
Compare
- This is done to avoid the `module-sync` condition.
2ebf48c
to
c944724
Compare
c944724
to
9de562c
Compare
9de562c
to
f9f283d
Compare
This fix is essential, Because in the latest version of Expo (Expo 53); It logs this warning uncountable times. I hope it is reviewed and merged soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the tsup changes (was this just a port to TS or bigger changes?) but the exports
field changes generally seem reasonable. This should at least be a minor bump, though.
"default": { | ||
"types": "./dist/index.d.ts", | ||
"default": "./dist/cjs/index.js" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going over one entry point in order here:
module-sync
is used to enablerequire(ESM)
if supported by the runtimemodule
aims at bundlersreact-native
, with ESM forimport
andCJS for
require`browser
with the ESM browser buildimport
with the ESM build- fallback to CJS, will probably only be hit in outdated runtimes that don't support
require(ESM)
Seems generally reasonable to me.
@@ -25,27 +25,137 @@ | |||
"module": "dist/redux-toolkit.legacy-esm.js", | |||
"main": "dist/cjs/index.js", | |||
"types": "dist/index.d.ts", | |||
"react-native": "dist/redux-toolkit.legacy-esm.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On top of that, for RN versions that support ESM, but not exports
, pointing at the legacy ESM build.
"types": "./../dist/query/index.d.ts", | ||
"exports": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping the exports
field should be okay - a bundler that supports exports
should look at the root-level package.json
.
@@ -4,16 +4,9 @@ | |||
"description": "", | |||
"type": "module", | |||
"module": "../dist/query/rtk-query.legacy-esm.js", | |||
"main": "../dist/query/cjs/index.js", | |||
"main": "./../dist/query/rtk-query.modern.mjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here be dragons - this switches from CJS to ESM. Probably be fine, since only outdated bundlers would look at this file at all, but pointing it out.
This PR: